adds count num sequences and tokens metric #346
Conversation
| import torch | ||
| import numpy as np | ||
|
|
||
| from fuse.eval.metrics.metrics_common import MetricPerBatchDefault | ||
|
|
||
|
|
||
| class MetricCountSeqAndTokens(MetricPerBatchDefault): |
There was a problem hiding this comment.
General question:
I'm not sure counting the sequences and tokens should be defined as metric. I don't have another suggestion it's just sounds weird :)
What do you think of that?
There was a problem hiding this comment.
It uses the metric mechanism, and it's ok to me that it just counts some stats.
| ) -> None: | ||
| """ | ||
| :param encoder_input: key to the encoder_input | ||
| :param ignore_index: token_id to ignore (not to count), typically pad token id |
There was a problem hiding this comment.
I think it should be able to support a list of token ids to ignore. Unless you want to enforce the user to ignore only the PAD one.
There was a problem hiding this comment.
I went with just one to be more efficient - typically we would like to just skip the padding.
| :param kwargs: additional super class arguments | ||
| """ | ||
| super().__init__( | ||
| seq_num="seq_num", # collect log_probs - output of _count_seq_and_tokens_update |
There was a problem hiding this comment.
obsolete comments in this line and the following one
|
|
||
| def _count_seq_and_tokens_update( | ||
| batch_dict: dict, | ||
| encoder_input_key: str, |
There was a problem hiding this comment.
encoder_input_key: Union[str, None] or
encoder_input_key: Optional[str]There was a problem hiding this comment.
It's. a must. Why optional?
|
|
||
| def _count_seq_and_tokens_compute( | ||
| self, | ||
| seq_num: List[np.ndarray], |
There was a problem hiding this comment.
seq_num will be a numpy array such that each entry represents a batch? If so, how often the metrics being calculate? each epoch?
I forgot these :)
There was a problem hiding this comment.
each sub epoch and each entry is a batch.
| self, | ||
| seq_num: List[np.ndarray], | ||
| token_num: List[np.ndarray], | ||
| ) -> float: |
| batch_dict: dict, | ||
| encoder_input_key: str, | ||
| ignore_index: Optional[int] = None, | ||
| ) -> Tuple[torch.Tensor, torch.Tensor]: |
There was a problem hiding this comment.
-> dict[str, Tensor]
|
Last comment, did you try to write a test for it? So we'll have it covered If time not permits maybe as a card on monday and we'll get to it later |
| ) -> None: | ||
| """ | ||
| :param encoder_input: key to the encoder_input | ||
| :param ignore_index: token_id to ignore (not to count), typically pad token id |
There was a problem hiding this comment.
I went with just one to be more efficient - typically we would like to just skip the padding.
| :param kwargs: additional super class arguments | ||
| """ | ||
| super().__init__( | ||
| seq_num="seq_num", # collect log_probs - output of _count_seq_and_tokens_update |
|
|
||
| def _count_seq_and_tokens_compute( | ||
| self, | ||
| seq_num: List[np.ndarray], |
There was a problem hiding this comment.
each sub epoch and each entry is a batch.
|
|
||
| def _count_seq_and_tokens_update( | ||
| batch_dict: dict, | ||
| encoder_input_key: str, |
There was a problem hiding this comment.
It's. a must. Why optional?
| batch_dict: dict, | ||
| encoder_input_key: str, | ||
| ignore_index: Optional[int] = None, | ||
| ) -> Tuple[torch.Tensor, torch.Tensor]: |
| self, | ||
| seq_num: List[np.ndarray], | ||
| token_num: List[np.ndarray], | ||
| ) -> float: |
|
Merging it to match inner-source code. |
No description provided.